Typing for DataArray/Dataset #2929

crusaderky · 2019-04-29T17:19:35Z

Status:

I'm generally not pleased with the amount of added verbosity. Happy to accept suggestions on how to improve.
Switching all variable names from str to Hashable. Without proper unit tests however (out of scope) non-string hashables are expected not to work most of the times. My preference would still be to stay limited on str...
DataArray done.
Dataset not done (except where it was hindering DataArray).
mypy passes with the only error "Mapping[...]" has no attribute "copy". This is due to the fact that I can't see a way to use typing.OrderedDict without breaking compatibility with python < 3.7.2.
py.test should be successful

@shoyer any early feedback is appreciated

pep8speaks · 2019-04-29T17:19:43Z

Hello @crusaderky! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-06-25 18:14:47 UTC

crusaderky · 2019-04-29T17:30:26Z

One of the many reasons why I'm strongly against generic Hashables instead of strings is that I expect user code all over the world to have string-specific functions and methods applied to variable names and dimensions.
e.g.

[d for d in arr.dims if d.startswith('foo')]

The above will produce an error in mypy, and the user will be forced to either disable the test or verbosely convince mypy with a typing.cast that actually in his project he never uses non-string dims.

max-sixty · 2019-04-29T18:36:02Z

Thanks @crusaderky , this looks great!

Re:

Switching all variable names from str to Hashable. Without proper unit tests however (out of scope) non-string hashables are expected not work most of the times. My preference would still be to stay limited on str...

Curious on others' thoughts. I would vote to allow Hashable for data variable names in Datasets - similar to other Mappings - but less so for dimension names in arrays. (Not opposed to it per se, open to instances where that would be helpful)

Conflicts: xarray/core/dataarray.py

crusaderky · 2019-05-08T02:45:57Z

DataArray finished; unit tests (hopefully) successful. Will now move to Dataset.
Would appreciate an official, final decision from @shoyer et. al. on Hashable vs. str before I continue - also in light of all the hacks Hashable forced me to go through.

…to annotations

shoyer · 2019-05-08T18:02:25Z

@crusaderky let's discuss that over in #2292. For now, I would try to defer making a decision on non-string dimension/variable names, even though that means we will have less informative type annotations.

# Conflicts: # xarray/core/dataarray.py # xarray/core/dataset.py # xarray/tests/__init__.py

max-sixty · 2019-06-14T22:45:05Z

@crusaderky I finished this off as best I could. Tests pass though needs a review. Here's the code: https://github.com/max-sixty/xarray/tree/annotations

(I tried pasting a patch, but I my git-fu isn't enough to exclude all the master code given there's a merge commit in there)

crusaderky · 2019-06-15T20:27:54Z

@max-sixty thanks for the work! I'll go through it ASAP

crusaderky · 2019-06-16T16:15:57Z

@max-sixty I merged your branch in and did some tweaks to the OrderedDict's.
Now moving to add more typing info to dataset.py...

max-sixty · 2019-06-16T21:05:09Z

xarray/core/dataarray.py

+            - mapping {coord name: DataArray}
+            - mapping {coord name: Variable}
+            - mapping {coord name: (dimension name, array-like)}
+            - mapping {coord name: (tuple of dimension names, array-like)}


max-sixty · 2019-06-16T21:13:32Z

Great, thanks @crusaderky ! Let me know if I can do anything to help!

max-sixty · 2019-06-24T18:24:10Z

Just fixed some more merge conflicts over at https://github.com/max-sixty/xarray/tree/annotations.

Thoughts on merging this now and continuing off master?

dcherian · 2019-06-24T18:34:40Z

Thoughts on merging this now and continuing off master?

+1 else the merge conflicts will just pile up.

crusaderky · 2019-06-24T19:32:36Z

Sorry I didn't have time last week - I'll try merging into the pr ASAP. I'm in favour of merging to mainline too

max-sixty · 2019-06-24T20:02:54Z

Cool, I think it should be a straight fast-forward merge from my branch to yours, and then an easy merge to master

xarray/core/dataarray.py

xarray/core/dataset.py

max-sixty · 2019-06-25T04:14:22Z

Great, I've resolved those comments @shoyer , thanks: https://github.com/max-sixty/xarray/tree/annotations

@crusaderky we should be ready to go - maybe take a glance over to ensure you're happy.

I added a whatsnew with both of us - hope that's OK

shoyer · 2019-06-25T14:59:36Z

@max-sixty do you want to push those changes to @crusaderky's branch? As a maintainer for xarray, I think you should have push permissions.

max-sixty · 2019-06-25T15:29:05Z

Ah it works! Didn't know permissions transferred to forks.

@crusaderky hope that's OK, not wanting to step on your toes here...

shoyer · 2019-06-25T14:57:27Z

xarray/core/dataarray.py

-                 attrs=None, encoding=None, indexes=None, fastpath=False):
+    def __init__(self, data: Any,
+                 coords: Union[
+                     Sequence[Tuple[Hashable, Any]],


This is still not quite right.

Erm, in the latest it's just Tuple, is that OK? (I think maybe I pushed an older one first and you looked over that)

Tuple should be fine, but it looks like that commit isn't on this branch yet

shoyer · 2019-06-25T16:30:22Z

xarray/core/dataset.py

+                 data_vars: Optional[Mapping[Hashable, Union[
+                     'DataArray', Variable,
+                     Tuple[Hashable, Any],
+                     Tuple[Tuple[Hashable, ...], Any]]]] = None,


This one also can handle any tuples args that you can use like Variable(*args) (maybe we could define a VariableArgs alias of some sort?)

Yes I added a comment re VariableArgs, def would be good to move some of these long definitions into a single place (but if you're OK with leaving until the next PR let's do that)

shoyer · 2019-06-25T16:32:53Z

By the way, I think I can write a script based on pep8speaks that writes/updates GitHub comments from within from Travis or Azure Pipelines. We could use that for surfacing mypy results in pull requests. I will probably give that a try over the next few days....

max-sixty · 2019-06-25T16:46:33Z

By the way, I think I can write a script based on pep8speaks that writes/updates GitHub comments from within from Travis or Azure Pipelines. We could use that for surfacing mypy results in pull requests. I will probably give that a try over the next few days....

That would be v cool. I'm kinda surprised something like pep8speaks doesn't do this already tbh...

shoyer · 2019-06-25T16:52:10Z

By the way, I think I can write a script based on pep8speaks that writes/updates GitHub comments from within from Travis or Azure Pipelines. We could use that for surfacing mypy results in pull requests. I will probably give that a try over the next few days....

That would be v cool. I'm kinda surprised something like pep8speaks doesn't do this already tbh...

I was considering adding this into pep8speaks, but I think this fundamentally incompatible with pep8speak's design. It runs a handful of pre-specified linters, without actually installing/running user code. But something like mypy needs to actually have xarray and its dependencies installed to work.

max-sixty · 2019-06-25T17:02:37Z

I was considering adding this into pep8speaks, but I think this fundamentally incompatible with pep8speak's design. It runs a handful of pre-specified linters, without actually installing/running user code. But something like mypy needs to actually have xarray and its dependencies installed to work.

Yes, good point.

I haven't spent much time with the GitHub Checks, but potentially could be a good fit - one issue with the simpler "Update the comment" model is that it doesn't encode to GH whether it's safe to merge, so projects need to duplicate the test in CI

xarray/core/dataarray.py

max-sixty · 2019-06-25T21:31:36Z

(AppVeyor fail looks unrelated)

shoyer · 2019-06-25T22:03:56Z

Thanks @max-sixty and @crusaderky !

max-sixty · 2019-06-25T22:23:21Z

Thanks @crusaderky !

typing for DataArray/Dataset (incomplete)

57ddd54

Guido Imperiale added 4 commits May 8, 2019 02:48

Merge branch 'master'

2db5fa3

Conflicts: xarray/core/dataarray.py

Finished dataarray.py

5b380ab

Bugfixes

4893640

Bugfixes

a784bc3

Guido Imperiale added 3 commits May 8, 2019 17:13

trivial

134bd5b

Merge branch 'annotations' of https://github.com/crusaderky/xarray in…

5dcbfce

…to annotations

Dataset (partial)

8746dc2

gimperiale mentioned this pull request May 9, 2019

Support non-string dimension/variable names #2292

Closed

Guido Imperiale and others added 6 commits May 9, 2019 10:33

Bugfixes

db35082

Minor tweaks

4cff627

Merge branch 'master' into annotations

0b748bf

final changes to dataset & dataarray for mypy

5894ea2

Merge branch 'master' into annotations

922cf2c

# Conflicts: # xarray/core/dataarray.py # xarray/core/dataset.py # xarray/tests/__init__.py

mypy str conflict

8857765

remove temp ptw setting

603005c

max-sixty mentioned this pull request Jun 15, 2019

Check types in travis #3024

Merged

crusaderky added 3 commits June 16, 2019 16:13

Merge remote-tracking branch 'max-sixty/annotations' into annotations

d95dc2a

Merge remote-tracking branch 'upstream/master' into annotations

400bc93

OrderedDict-related fixes

e601bfe

max-sixty reviewed Jun 16, 2019

View reviewed changes

shoyer reviewed Jun 24, 2019

View reviewed changes

max-sixty added 5 commits June 24, 2019 20:49

Hashable -> Any for Dicts

0bb3472

DataArray names as Hashable

f9864ba

resolve some __default vs None issues

a9b748b

whats-new

2bc87eb

just specify Tuple at the moment

d2cb1e1

remove unneeded cast, make arg formatting more black-like

e5e381b

shoyer requested changes Jun 25, 2019

View reviewed changes

adjust init args based on @shoyer review

9b75e6a

max-sixty reviewed Jun 25, 2019

View reviewed changes

xarray/core/dataarray.py Show resolved Hide resolved

max-sixty added 2 commits June 25, 2019 13:57

add Tuple[Tuple] to dataset init types

7bd558f

tuples not all the way down

cbe0e7b

shoyer merged commit d3f6db9 into pydata:master Jun 25, 2019

crusaderky changed the title ~~WIP: typing for DataArray/Dataset~~ Typing for DataArray/Dataset Jun 30, 2019

crusaderky deleted the annotations branch June 30, 2019 10:08

trevormorgan mentioned this pull request Jan 20, 2021

feature: add native xarray support and concatenation of DataFrames/DataArrays microsoft/Qcodes#2507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typing for DataArray/Dataset #2929

Typing for DataArray/Dataset #2929

crusaderky commented Apr 29, 2019 •

edited

Loading

pep8speaks commented Apr 29, 2019 •

edited

Loading

crusaderky commented Apr 29, 2019

max-sixty commented Apr 29, 2019

crusaderky commented May 8, 2019

shoyer commented May 8, 2019

max-sixty commented Jun 14, 2019

crusaderky commented Jun 15, 2019

crusaderky commented Jun 16, 2019

max-sixty Jun 16, 2019

max-sixty commented Jun 16, 2019

max-sixty commented Jun 24, 2019

dcherian commented Jun 24, 2019

crusaderky commented Jun 24, 2019

max-sixty commented Jun 24, 2019

max-sixty commented Jun 25, 2019 •

edited

Loading

shoyer commented Jun 25, 2019

max-sixty commented Jun 25, 2019

shoyer Jun 25, 2019

max-sixty Jun 25, 2019

shoyer Jun 25, 2019

shoyer Jun 25, 2019

max-sixty Jun 25, 2019

shoyer commented Jun 25, 2019

max-sixty commented Jun 25, 2019

shoyer commented Jun 25, 2019

max-sixty commented Jun 25, 2019

max-sixty commented Jun 25, 2019

shoyer commented Jun 25, 2019

max-sixty commented Jun 25, 2019

Typing for DataArray/Dataset #2929

Typing for DataArray/Dataset #2929

Conversation

crusaderky commented Apr 29, 2019 • edited Loading

pep8speaks commented Apr 29, 2019 • edited Loading

Comment last updated at 2019-06-25 18:14:47 UTC

crusaderky commented Apr 29, 2019

max-sixty commented Apr 29, 2019

crusaderky commented May 8, 2019

shoyer commented May 8, 2019

max-sixty commented Jun 14, 2019

crusaderky commented Jun 15, 2019

crusaderky commented Jun 16, 2019

max-sixty Jun 16, 2019

Choose a reason for hiding this comment

max-sixty commented Jun 16, 2019

max-sixty commented Jun 24, 2019

dcherian commented Jun 24, 2019

crusaderky commented Jun 24, 2019

max-sixty commented Jun 24, 2019

max-sixty commented Jun 25, 2019 • edited Loading

shoyer commented Jun 25, 2019

max-sixty commented Jun 25, 2019

shoyer Jun 25, 2019

Choose a reason for hiding this comment

max-sixty Jun 25, 2019

Choose a reason for hiding this comment

shoyer Jun 25, 2019

Choose a reason for hiding this comment

shoyer Jun 25, 2019

Choose a reason for hiding this comment

max-sixty Jun 25, 2019

Choose a reason for hiding this comment

shoyer commented Jun 25, 2019

max-sixty commented Jun 25, 2019

shoyer commented Jun 25, 2019

max-sixty commented Jun 25, 2019

max-sixty commented Jun 25, 2019

shoyer commented Jun 25, 2019

max-sixty commented Jun 25, 2019

crusaderky commented Apr 29, 2019 •

edited

Loading

pep8speaks commented Apr 29, 2019 •

edited

Loading

max-sixty commented Jun 25, 2019 •

edited

Loading